Add ARC MPU v3 & v6 support #37871
Add ARC MPU v3 & v6 support #37871
Conversation
github-actions
bot
added
area: API
|
related issue #37309 |
include/arch/arc/arch.h
Outdated
There was a problem hiding this comment.
Maybe
#if (CONFIG_ARC_MPU_VER == 4) || (CONFIG_ARC_MPU_VER == 6)
?
There was a problem hiding this comment.
ZEPHYR_ARCH_ARC_CORE_MPU_ARC_MPU_V6_INTERNAL_H_ (see v6)
There was a problem hiding this comment.
Why mention MPUv2 in code for MPUv6?
There was a problem hiding this comment.
This one fits in 100 symbol per line limit so let's keep it in one line.
There was a problem hiding this comment.
Hmm, is it really about MPUv2?
There was a problem hiding this comment.
This one fits in 100 symbol per line limit so let's keep it in one line.
There was a problem hiding this comment.
This one fits in 100 symbol per line limit so let's keep it in one line.
There was a problem hiding this comment.
This one fits in 100 symbol per line limit so let's keep it in one line.
There was a problem hiding this comment.
This one fits in 100 symbol per line limit so let's keep it in one line.
There was a problem hiding this comment.
This one fits in 100 symbol per line limit so let's keep it in one line.
soc/arc/snps_nsim/Kconfig
Outdated
There was a problem hiding this comment.
Hmm, Is it correct? We don't have MPU in this NSIM SoC configuration.
YuguoWH
force-pushed
the
topic-mpu-v6
branch
5 times, most recently
from
5e0f2ae to
36ad8bd
Compare
There was a problem hiding this comment.
Frankly I don't like these cryptic numbers. I guess these are some groups of bits shifted left?
Look at BIT_MASK() in https://github.com/zephyrproject-rtos/zephyr/blob/main/include/sys/util_macro.h#L68. Then we'll get something like BIT_MASK(xxx) << yyy which is much more readable.
There was a problem hiding this comment.
Maybe BIT(x) and ~BIT(x) correspondigly?
There was a problem hiding this comment.
As we use r_index only in loop probably it's better to define it in loop itself:
for (int r_index = 0; r_index < get_num_regions(); r_index++) {
There was a problem hiding this comment.
I don't think we need to split definition and declaration here. Let's combine it:
/* comment */
int r_index = num_regions - mpu_config.num_regions;
There was a problem hiding this comment.
We don't exceed the Zephyr line limit here (100 symbols per line), please keep it in one line.
Please check rest of the code - we have a lot of places where we can put the whole expression in one line. It improves code readability a lot.
There was a problem hiding this comment.
Should we move those magic numbers to some defines / constants?
There was a problem hiding this comment.
I guess it would be enough to also use much simpler understood BIT_MASK(2) & BIT_MASK(3) << 9 here.
There was a problem hiding this comment.
Still It would be better to have all this shift values named. For example we can use "AUX_MPU_REGxx_FIELDxx_SHIFT"
There was a problem hiding this comment.
As we use i only in loop probably it's better to define it in loop itself:
for (uint32_t i = 0U; i < r_index; i++) {
489f934 to
43ac849
Compare
There was a problem hiding this comment.
So you don't like #define AUX_MPU_RDP_ATTR_MASK (BIT_MASK(6) << 3)? ;)
There was a problem hiding this comment.
And here #define AUX_MPU_RDP_SIZE_MASK (BIT_MASK(3) << 9 | BIT_MASK(2))?
There was a problem hiding this comment.
I guess it would be enough to also use much simpler understood BIT_MASK(2) & BIT_MASK(3) << 9 here.
|
@YuguoWH could you please also add the nSIM board with v6 MPU so we can easily test it in our verification / manually? |
YuguoWH
force-pushed
the
topic-mpu-v6
branch
2 times, most recently
from
f943954 to
8cdf600
Compare
There was a problem hiding this comment.
Why not use BIT_MASK here as well as in the next line?
There was a problem hiding this comment.
then it would be:
#define AUX_MPU_RDP_REGION_SIZE(size) (((size - 1) & BIT_MASK(2)) | \ (((size - 1) & (BIT_MASK(3) << 2)) << 7))
I think it looks more complex...
Add support of ARC mpu version 3 which can have region size down to 32 bytes Signed-off-by: Yuguo Zou <[email protected]>
|
Hi @evgeniy-paltsev , PR rebased, thanks. |
Add support of ARC mpu v6 * minimal region size down to 32 bytes * maximal region number up to 32 * not support uncacheable region and volatile uncached region * clean up mpu code for better readablity Signed-off-by: Yuguo Zou <[email protected]>
We add support of mpu v6 therefore it is needed to have a board to validate that feature. This commit add a new HS nsim simulator which supports mpu v6. Signed-off-by: Yuguo Zou <[email protected]>
|
@nashif could you please pick this one up for v2.7? |
| config SOC_NSIM_HS_MPUV6 | ||
| bool "Synopsys ARC HS with MPU v6 in nSIM" | ||
| select CPU_HAS_MPU | ||
| select CPU_HAS_FPU |
There was a problem hiding this comment.
I keep it same with other NSIM_HS boards, so FPU is enabled
6 participants
Adding ARC MPU v3 and v6 support.
The v3 uses same logic with v2 and the only difference is to support 32 bytes minimal region size.
The v6 support 32 bytes, and it can have up to 32 regions. The regions are divided into 2 banks and switched by set/unset RB bit in MPU_EN aux register
This PR also introduces a new nsim hs board simulator which can validate/test with mpu v6.